Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add link to declarative shadow dom opt-in #300

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mfreed7
Copy link

@mfreed7 mfreed7 commented Nov 24, 2020

This PR goes along with the two in HTML and DOM to add declarative Shadow DOM to the spec. This PR goes along with the discussion at #912, to opt-in to the fragment parser for declarative Shadow DOM.


Preview | Diff

@annevk
Copy link
Member

annevk commented Nov 30, 2020

As I mentioned there, I don't think we should add this feature to this API. XMLHttpRequest is essentially frozen and new functionality should go into fetch() if applicable, but I don't think this is. I didn't see pushback there from you so maybe you missed that feedback?

@mfreed7
Copy link
Author

mfreed7 commented Dec 1, 2020

I agree, and I didn’t miss that feedback. This change explicitly sets the “include shadow roots” flag to “deny”, so that XHR cannot be used to parse DSD content. Without this change to XHR, it will allow DSD parsing. So effectively, this is “no change”.

@annevk
Copy link
Member

annevk commented Dec 1, 2020

I missed that, is there a reason "deny" is not the default, given that it's "unsafe"?

@mfreed7
Copy link
Author

mfreed7 commented Dec 2, 2020

I missed that, is there a reason "deny" is not the default, given that it's "unsafe"?

I'm open to suggestions on how I implemented this in the HTML spec (and in code), but as it stands, the "include shadow roots" flag is tri-state. It can be unset, or explicitly "allow" or "deny". The reason is that for fragment parsing, unset means "don't allow" DSD content. But for non-fragment parsing, unset means "allow" DSD content. The explicit cases allow that default behavior to be overridden, e.g. here for XHR, where the XHR document is parsed with a non-fragment parser, but we still don't want to allow DSD content. Or the opposite case for DOMParser with the includeShadowRoots flag set to true, we want to explicitly allow DSD content even for the fragment parser.

@annevk
Copy link
Member

annevk commented Dec 2, 2020

Interesting, I guess I need to look at the specific PRs, but it seems that it could be a boolean everywhere that defaults to false, and when we invoke the main parser for documents we set it to true.

@mfreed7
Copy link
Author

mfreed7 commented Dec 4, 2020

Interesting, I guess I need to look at the specific PRs, but it seems that it could be a boolean everywhere that defaults to false, and when we invoke the main parser for documents we set it to true.

Ok. I'll be interested to hear your suggestions for implementing it that way. I found in the implementation that there are multiple places we invoke the parser, so this was the least overhead approach to avoid touching most of them. But always open to improvements here!

Base automatically changed from master to main January 15, 2021 07:39
@annevk
Copy link
Member

annevk commented Dec 8, 2022

This is obsolete now, right @mfreed7?

@mfreed7
Copy link
Author

mfreed7 commented Dec 16, 2022

This is obsolete now, right @mfreed7?

No, I think we still need this, to ensure XHR doesn't parse DSD content. It does need an update, since "include shadow roots state" has been renamed.

@annevk
Copy link
Member

annevk commented Dec 16, 2022

But isn't the default of this state deny? Oh, I guess it also has unset, but I still don't really understand that third state.

@mfreed7
Copy link
Author

mfreed7 commented Jan 13, 2023

But isn't the default of this state deny? Oh, I guess it also has unset, but I still don't really understand that third state.

The default is "unset" which is interpreted as "allow" for document parsing and "deny" for synchronous parsing.

@annevk
Copy link
Member

annevk commented Jan 14, 2023

Can we make it deny and have page loading overwrite it? Usually when we have tri-states there are actually three different behaviors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants